Odbc driver feature#596
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an initial YDB ODBC driver implementation (handles, connections, statements, attribute handling, basic metadata queries, and ODBC escape rewriting) along with unit/integration tests and CMake build integration so the driver can be built as part of the SDK.
Changes:
- Introduces
ydb-odbcshared library with core ODBC entrypoints and supporting utilities (conversion, diagnostics, cursors, attribute parsing, escape rewriting). - Adds ODBC unit tests (escape rewriting, SQL LIKE matching, param conversion) and integration tests (basic query flow, env/conn/stmt attrs).
- Integrates ODBC build/test targets into the SDK CMake configuration (option, preset, external ODBC dependency, test helper).
Reviewed changes
Copilot reviewed 54 out of 54 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/library/operation_id/CMakeLists.txt | Adds proto library dependency to operation_id unit test. |
| odbc/tests/unit/sql_like_ut.cpp | Unit tests for SQL LIKE pattern matching helper. |
| odbc/tests/unit/escape_ut.cpp | Unit tests for ODBC escape sequence rewriting. |
| odbc/tests/unit/convert_ut.cpp | Unit tests for ODBC parameter conversion to YDB params. |
| odbc/tests/unit/CMakeLists.txt | Defines ODBC unit test targets and include paths. |
| odbc/tests/integration/test_utils.h | Common ODBC integration test helpers (connect, error formatting). |
| odbc/tests/integration/stmt_attr_it.cpp | Integration tests for statement attributes + metadata-id behavior. |
| odbc/tests/integration/env_it.cpp | Integration tests for environment-level transactions (SQLEndTran). |
| odbc/tests/integration/CMakeLists.txt | Defines ODBC integration test targets. |
| odbc/tests/integration/basic_it.cpp | Integration tests for simple query, parameters, and column binding. |
| odbc/tests/integration/attr_it.cpp | Integration tests for env/connection attribute behavior. |
| odbc/tests/CMakeLists.txt | Adds ODBC tests subdirectories. |
| odbc/src/utils/util.h | Declares helper for converting ODBC strings to std::string. |
| odbc/src/utils/util.cpp | Implements ODBC string conversion helper. |
| odbc/src/utils/types.h | Declares YDB-type to ODBC-metadata helpers. |
| odbc/src/utils/types.cpp | Implements (partially) YDB-type to ODBC-metadata helpers. |
| odbc/src/utils/sql_like.h | Adds inline SQL LIKE matcher used by metadata filtering. |
| odbc/src/utils/escape.h | Declares ODBC escape rewrite function. |
| odbc/src/utils/escape.cpp | Implements ODBC escape rewriting (+ CONVERT-to-CAST rewriting). |
| odbc/src/utils/error_manager.h | Adds diagnostic record storage + exception-to-diagnostic helpers. |
| odbc/src/utils/error_manager.cpp | Implements diagnostic record mapping and retrieval. |
| odbc/src/utils/diag.h | Provides small helpers for standard SQLSTATE diagnostics. |
| odbc/src/utils/cursor.h | Declares cursor abstraction for exec/virtual cursors. |
| odbc/src/utils/cursor.cpp | Implements exec cursor (stream iterator) and virtual cursor (in-memory table). |
| odbc/src/utils/convert.h | Declares parameter and column conversion helpers. |
| odbc/src/utils/convert.cpp | Implements ODBC param conversion registry + YDB->ODBC column conversion. |
| odbc/src/utils/bindings.h | Defines bound parameter/column structs and binding filler interface. |
| odbc/src/utils/attr.h | Declares attribute read/write helpers (string + integer tokens). |
| odbc/src/utils/attr.cpp | Implements attribute string read/write and truncation diagnostics. |
| odbc/src/statement.h | Declares statement handle (prepare/execute/fetch/metadata/attrs). |
| odbc/src/statement.cpp | Implements statement execution, binding, metadata queries, and pattern scanning. |
| odbc/src/statement_attr.h | Declares statement attribute storage + get/set API. |
| odbc/src/statement_attr.cpp | Implements statement attribute get/set logic. |
| odbc/src/odbc_driver.cpp | Implements ODBC C entrypoints wiring to internal handle objects. |
| odbc/src/environment.h | Declares environment handle (attrs, connection registry, EndTran). |
| odbc/src/environment.cpp | Implements environment attrs and env-level EndTran across connections. |
| odbc/src/connection.h | Declares connection handle (connect/attrs/tx/session/client mgmt). |
| odbc/src/connection.cpp | Implements connection string parsing, driver pooling, tx control, catalog routing. |
| odbc/src/connection_attr.h | Declares connection attribute storage + catalog routing helpers. |
| odbc/src/connection_attr.cpp | Implements connection attributes (autocommit, access mode, isolation, catalog). |
| odbc/README.md | Adds build/config/usage documentation for the ODBC driver. |
| odbc/odbcinst.ini | Provides installed driver registration template. |
| odbc/odbc.ini | Provides sample DSN configuration template. |
| odbc/examples/scheme/main.cpp | Example program using SQLTables to enumerate tables. |
| odbc/examples/scheme/CMakeLists.txt | Builds scheme example and injects built driver path macro. |
| odbc/examples/CMakeLists.txt | Adds example subdirectories. |
| odbc/examples/basic/main.cpp | Example program using params and result fetching/binding. |
| odbc/examples/basic/CMakeLists.txt | Builds basic example and injects built driver path macro. |
| odbc/CMakeLists.txt | Builds/install ydb-odbc and adds tests/examples subdirs. |
| CMakePresets.json | Enables YDB_SDK_ODBC in presets. |
| CMakeLists.txt | Adds YDB_SDK_ODBC option and includes odbc/ when enabled. |
| cmake/testing.cmake | Adds add_odbc_test() helper when ODBC is enabled. |
| cmake/external_libs.cmake | Adds find_package(ODBC REQUIRED) under YDB_SDK_ODBC. |
| cmake/common.cmake | Extends _ydb_sdk_add_library options and changes global lib creation helper. |
Comments suppressed due to low confidence (1)
cmake/common.cmake:126
- add_global_library_for() now calls _ydb_sdk_add_library(${TgtName} STATIC ...), but _ydb_sdk_add_library() does not recognize a STATIC option. As a result, the created library type depends on CMake defaults/BUILD_SHARED_LIBS instead of being explicitly STATIC, which can break link behavior (whole-archive) and packaging. Please either teach _ydb_sdk_add_library() to accept STATIC, or change add_global_library_for() to call add_library(${TgtName} STATIC) (or _ydb_sdk_add_library with an explicit mode it supports).
function(add_global_library_for TgtName MainName)
_ydb_sdk_add_library(${TgtName} STATIC ${ARGN})
if(APPLE)
target_link_options(${MainName} INTERFACE "SHELL:-Wl,-force_load,$<TARGET_FILE:$<INSTALL_INTERFACE:YDB-CPP-SDK::>${TgtName}>")
else()
target_link_options(${MainName} INTERFACE "SHELL:-Wl,--whole-archive $<TARGET_FILE:$<INSTALL_INTERFACE:YDB-CPP-SDK::>${TgtName}> -Wl,--no-whole-archive")
endif()
add_dependencies(${MainName} ${TgtName})
target_link_libraries(${MainName} INTERFACE ${TgtName})
endfunction()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 63 out of 63 changed files in this pull request and generated 16 comments.
Comments suppressed due to low confidence (1)
cmake/common.cmake:123
add_global_library_for()now calls_ydb_sdk_add_library(${TgtName} STATIC ${ARGN}), but_ydb_sdk_add_library()doesn’t handle theSTATICkeyword or forward${ARGN}as sources. This effectively creates an (empty) library target and drops the intended source list, which will break builds if/whenadd_global_library_foris used. Consider either restoringadd_library(${TgtName} STATIC ${ARGN})here, or extending_ydb_sdk_add_libraryto accept a library type + source list (or calltarget_sources()with the unparsed args).
function(add_global_library_for TgtName MainName)
_ydb_sdk_add_library(${TgtName} STATIC ${ARGN})
if(APPLE)
target_link_options(${MainName} INTERFACE "SHELL:-Wl,-force_load,$<TARGET_FILE:$<INSTALL_INTERFACE:YDB-CPP-SDK::>${TgtName}>")
else()
target_link_options(${MainName} INTERFACE "SHELL:-Wl,--whole-archive $<TARGET_FILE:$<INSTALL_INTERFACE:YDB-CPP-SDK::>${TgtName}> -Wl,--no-whole-archive")
endif()
EndTran for env + tests
Combine connection attribute routing and error-localization updates into one focused commit while keeping driver pool changes separate. Made-with: Cursor
90c479b to
835c5d1
Compare
| std::optional<NQuery::TExecuteQueryPart> prefetchedPart) | ||
| : BindingFiller_(bindingFiller) | ||
| , Iterator_(std::move(iterator)) | ||
| , PrefetchedPart_(std::move(prefetchedPart)) |
There was a problem hiding this comment.
А не проще вызвать 1 раз Fetch, а не передавать PrefetchedPart_
There was a problem hiding this comment.
Нет, fetch() не только читает стрим, но и переходит на первую строку (TryNextRow). Если вызвать его в конце Execute, приложение на первом SQLFetch получит вторую строку.
There was a problem hiding this comment.
Я не буквально имею в виду. Тут вот какие замечания:
- prefetched part частично дублирует логику fetch
- Мы размазываем зону логику вычитывания результата на вызывающую сторону
| add_ydb_test(NAME odbc-escape_ut GTEST | ||
| SOURCES | ||
| escape_ut.cpp | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/../../src/utils/escape.cpp |
There was a problem hiding this comment.
А мы не можем просто позависить на ydb-odbc?
| @@ -277,31 +277,21 @@ | |||
| std::optional<std::string> fetched; | |||
| const NYdb::TStatus status = client->RetryQuerySync( | |||
There was a problem hiding this comment.
Можем поставить флаг идемпотентности, так ретраить лучше будет
| return NYdb::TStatus(EStatus::SUCCESS, NYdb::NIssue::TIssues()); | ||
| }); | ||
|
|
||
| if (status.IsSuccess() && fetched && !fetched->empty()) { |
There was a problem hiding this comment.
Может тут лучше ошибку выдавать, если получить не смогли версию?
| add_subdirectory(examples) | ||
| add_subdirectory(tests) | ||
|
|
||
| install(FILES |
There was a problem hiding this comment.
А не для тестов генерить не надо?
|
|
||
| For `SQLConnect("YDB", ...)`, `isql -v YDB`, or `Driver=YDB`. | ||
|
|
||
| **`odbcinst.ini`** — driver registration. Section `[YDB]` is the driver name used as `Driver=YDB` in connection strings and DSNs. `Driver` and `Setup` are the full path to `libydb-odbc.so`. Use `/etc/odbcinst.ini`, a file in `/etc/odbcinst.d/`, or set `ODBCSYSINI` to the directory that contains `odbcinst.ini`. |
There was a problem hiding this comment.
В моем понимании odbcinst.ini - генерится, odbc.ini - пишет пользователь
No description provided.